Skip to content

Refactor to run all calculations in separate directories #220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 114 commits into from
Oct 10, 2024
Merged

Refactor to run all calculations in separate directories #220

merged 114 commits into from
Oct 10, 2024

Conversation

elinscott
Copy link
Collaborator

@elinscott elinscott commented May 7, 2024 •
edited
Loading

Summary

This refactor of the code contains several changes designed to make koopmans more robust and amenable to integration with AiiDA. Namely...

  • each calculation is run in a separate directory following a regular pattern
  • there are no shared tmp directories
  • file reading/writing/moving is done in an abstract way (see the new FilePointer class) so that it will be possible to "move" files on a remote server
  • the introduction of a Process class to make python operations on files etc. more standardized (and able to be executed remotely)

These changes to treat calculations more like pure functions, and the introduction of a Process class also starts us in the direction of following CWL's design pattern more closely. In the long term, we would like to be as close as possible to CWL (perhaps even with composite workflows being able to be written in CWL).

Other changes

  • the machine-learning workflows have been simplified
  • the output of koopmans markdown-compliant, making the output files easier to read for humans
  • .kwf files have been replaced by .pkl files (removing the responsibility of writing Workflows to file from custom code and using the widely-used dill package instead)

@pep8speaks
Copy link

pep8speaks commented May 7, 2024 •
edited
Loading

Hello @elinscott! Thanks for updating this PR.

Line 276:121: E501 line too long (162 > 120 characters)

Line 49:121: E501 line too long (159 > 120 characters)

Line 197:121: E501 line too long (121 > 120 characters)
Line 208:121: E501 line too long (123 > 120 characters)
Line 468:121: E501 line too long (133 > 120 characters)
Line 472:121: E501 line too long (121 > 120 characters)

Line 29:75: E226 missing whitespace around arithmetic operator

Line 71:67: E226 missing whitespace around arithmetic operator

Line 59:121: E501 line too long (138 > 120 characters)
Line 66:121: E501 line too long (143 > 120 characters)
Line 221:121: E501 line too long (172 > 120 characters)
Line 224:121: E501 line too long (211 > 120 characters)
Line 398:121: E501 line too long (126 > 120 characters)
Line 415:121: E501 line too long (124 > 120 characters)

Line 149:23: E226 missing whitespace around arithmetic operator
Line 149:35: E226 missing whitespace around arithmetic operator
Line 149:47: E226 missing whitespace around arithmetic operator
Line 238:51: E226 missing whitespace around arithmetic operator
Line 257:34: E226 missing whitespace around arithmetic operator
Line 274:34: E226 missing whitespace around arithmetic operator
Line 305:34: E226 missing whitespace around arithmetic operator

Line 194:121: E501 line too long (123 > 120 characters)
Line 196:121: E501 line too long (122 > 120 characters)

Line 26:121: E501 line too long (123 > 120 characters)
Line 507:121: E501 line too long (129 > 120 characters)
Line 669:105: E226 missing whitespace around arithmetic operator
Line 700:121: E501 line too long (159 > 120 characters)

Line 48:9: E722 do not use bare 'except'

Line 65:47: E226 missing whitespace around arithmetic operator
Line 200:121: E501 line too long (126 > 120 characters)
Line 236:121: E501 line too long (128 > 120 characters)
Line 300:121: E501 line too long (124 > 120 characters)

Line 833:121: E501 line too long (125 > 120 characters)
Line 916:121: E501 line too long (186 > 120 characters)
Line 921:121: E501 line too long (138 > 120 characters)
Line 929:121: E501 line too long (150 > 120 characters)

Line 27:22: E226 missing whitespace around arithmetic operator
Line 40:22: E226 missing whitespace around arithmetic operator

Line 43:28: W605 invalid escape sequence '\A'

Line 16:39: E226 missing whitespace around arithmetic operator

Comment last updated at 2024-06-27 10:28:10 UTC

elinscott added 28 commits May 7, 2024 09:55
@elinscott
Updated pre-commit config
3f1432d
@elinscott
Autotidying
de31609
@elinscott
Adding link() method to workflows; WIP
1e65d7b
@elinscott
Disabling pDOS for the moment
9d6293e
@elinscott
Added Process to abstract file manipulation
ac625b9
@elinscott
Updated tests to reflect restructured wannier writing and reading
12a14e0
@elinscott
Updated test for merge_wannier_hr
d88939d
@elinscott
Reordering code so that each calculation is performed in its own dire…
fe0f879 
…ctory (WIP)
@elinscott
Refactoring DFPT workflow so that screening parameters is a subworkflow
c57f55c
@elinscott
Aesthetic tweaks to warnings and printed messages
9d05e46
@elinscott
Moving get_content and write_content to utils
cffee82
@elinscott
Minor edit to .bib
6b1f413
@elinscott
Merged Numpy v2.0 compatibility; implemented w2kc and ham in new fold…
d92f881 
…er framework
@elinscott
Minor bugfix
3e64d39
@elinscott
Minor tweaks to make refactored DFPT workflow work
62a5b04
@elinscott
WIP: converting UI from an ASE calculator to a Process.
0b0d022
@elinscott
Removed test for deprecated UI calculator
2117cb1
@elinscott
Change block subworkflow to only do one block; rejigged the way that …
775efec 
…workflow names work
@elinscott
Converting copying to symlinking where possible for kc_ham
50fb946
@elinscott
WIP: converting DSCF workflow to separate folders
d3fad4d
@elinscott
WIP: testing tutorial 2
a2dfd0e
@elinscott
WIP: converted tutorial_2 (bulk Si with DSCF)
0fbc60d
@elinscott
WIP: Tutorial 3 running (results not yet checked)
59a15e3
@elinscott
Make wannierization use more symlinks
65f9a1e
@elinscott
Making convergence and functional = all work; changing appearance of …
9373f05 
…printout
@elinscott
Beginning to tweak tests to work with new changes
8e6caa8
@elinscott
Further editing of ML workflows
642e08d
@elinscott
Tidied tutorlai 5 to use new train/test/predict options (docs still n…
6b05f1d 
…eed updating)
@elinscott elinscott changed the title Adding framework for parallel execution of calculators Refactor to run all calculations in separate directories Oct 7, 2024
@elinscott elinscott self-assigned this Oct 7, 2024
@elinscott elinscott marked this pull request as ready for review October 10, 2024 08:31
@elinscott elinscott requested a review from nscolonna October 10, 2024 08:32
nscolonna
nscolonna approved these changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@nscolonna nscolonna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the huge amount of work!

@elinscott elinscott merged commit 369ea5b into master Oct 10, 2024
7 checks passed
@elinscott elinscott deleted the implement_run_calculators branch October 10, 2024 12:00
@nscolonna
Copy link
Collaborator

nscolonna commented Oct 10, 2024 via email

@elinscott elinscott mentioned this pull request Oct 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nscolonna nscolonna nscolonna approved these changes

Assignees

@elinscott elinscott

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elinscott @pep8speaks @nscolonna